-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat(chalk-to-util-styletext): add workflow #256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat(chalk-to-util-styletext): add workflow #256
Conversation
|
don't miss to do a For dependency removal I'm asking the Codemod team. because on V0 when the tools was JS only and there was a simple thing to manage dep |
AugustinMauroy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some code style
|
@AugustinMauroy the PR is still a draft, no need to nitpick yet |
😅 oops I'm flash macqueen. Nop kidding just inattentive |
|
@AugustinMauroy I've pushed a few commits which I believe address some of the earlier comments. From the original Github issue, 7/8 test cases are passing. The one test case remaining is the complex chaining example. const styles = level === "error" ? chalk.red.bold : chalk.yellow;const styles =
level === "error"
? (text) => styleText(["red", "bold"], text)
: (text) => styleText("yellow", text);I have a rough idea/implementation locally for this, but it's a bit nasty and I'm not sure I like it. But I'm keen to get feedback on this initial approach and understand if there are possibly better ways of implementing. |
|
I'm going to review your current code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOW clean. About complex thing I didn't have any idea now I'm getting tired. But you do a great job here.
Missing tests/support about import.
const mychalk = require('chalk');const { red }. = require(-'chalk');maybe hardconst { red; foo }. = require(-"chalk");const foo = await import('chalk');const { red } = await import('chalk')import * as chalk from 'chalk'import { red } from 'chalk';import { red as colorRed 'chalk';
For dependcy par you I'll need custom thing (new step on workflow.yaml):
- step that have JSSG
child_process&fscapacity enabled - function that use FS to detect which package manager it's use (
npm,yarn,pnpm) - new function on package-json utility that removes dep
So jssg step will be architecture as: - get the package manager
- remove dep with the utility function
- run the good child process
for this part I can take over the pr to help you.
https://docs.codemod.com/cli/packages/building-workflows#steps
https://docs.codemod.com/jssg/security
…oll/userland-migrations into feat/chalk-styletext-migration
…oll/userland-migrations into feat/chalk-styletext-migration
|
LGTM for now the warning si good. Missing some piece that already in the todo. |
Here's my plan for the remaining work:
|
|
Okay nice ! |
|
the complex chaining handling is clever 👌🏻 |
…oll/userland-migrations into feat/chalk-styletext-migration
|
@AugustinMauroy I've pushed some commits that close off the remaining TODO's that were left from last week. I've also made an attempt at the dependency removal process that you mentioned above. I've also done a bit of testing with this implementation across some open source projects that use
I'm happy to take a stab at those in this PR, although the amount of files now is starting to grow. It may make sense to do in a follow up. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add unit test for this file. using node:test & node:assert
|
|
||
| registry: | ||
| access: public | ||
| visibility: public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| visibility: public | |
| visibility: public | |
| capabilities: | |
| - fs | |
| - child_process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AugustinMauroy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to take a stab at those in this PR, although the amount of files now is starting to grow. It may make sense to do in a follow up. WDYT?
Why my comment above we are good to land and support rest after
Related issue
Fixes #198
Test cases
const mychalk = require('chalk');const { red } = require('chalk');maybe hardconst { red; foo }. = require("chalk");const foo = await import('chalk');const { red } = await import('chalk')import * as chalk from 'chalk'import { red } from 'chalk';import { yellow as y } from 'chalk';